Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Small amendments for Hash#merge with IndifferentAccess #525

Merged
merged 1 commit into from Jun 10, 2020

Conversation

yogeshjain999
Copy link
Contributor

  1. Use indifferent_writer in convert! so that when
    indifferent_writer, convert_key or indifferent_value is
    overridden in included class, merge can use those.

  2. convert! was calling twice if other hash was lacking
    indifference. IndifferentAccess.inject! already does conversion.

I've not written specs because both changes doesn't seem to be either fix or feature. But let me know if needed. 馃

@dangerpr-bot
Copy link

dangerpr-bot commented Jun 6, 2020

1 Warning
鈿狅笍 Unless you鈥檙e refactoring existing code or improving documentation, please update CHANGELOG.md.

Here's an example of a CHANGELOG.md entry:

* [#525](https://github.com/hashie/hashie/pull/525): Small amendments for hash#merge with indifferentaccess - [@yogeshjain999](https://github.com/yogeshjain999).

Generated by 馃毇 Danger

@dblock
Copy link
Member

dblock commented Jun 8, 2020

How did you find this? I think the first one should be testable, since if you override any of these methods in the included class merge would miss them, no?

@yogeshjain999
Copy link
Contributor Author

@dblock Right. How about the spec I've added ?
I found this when I was overriding writers in our gem for some custom needs.

1. Use `indifferent_writer` in `convert!` so that when
`indifferent_writer`, `convert_key` or `indifferent_value` is
overridden in included class, `merge` can use those.

2. `convert!` was calling twice if `other` hash was lacking
indifference. `IndifferentAccess.inject!` already does conversion.
@dblock
Copy link
Member

dblock commented Jun 10, 2020

Love the use of grumpy and indifferent cat. Merging.

@dblock dblock merged commit c95b261 into hashie:master Jun 10, 2020
@dblock
Copy link
Member

dblock commented Jun 10, 2020

Now that I think about this I think there was a changelog line here, "Use indifferent_writer in convert!", it exposes a way to override it, but NBD.

@yogeshjain999
Copy link
Contributor Author

Ah right 馃槃

@yogeshjain999 yogeshjain999 deleted the indifferent-convert-change branch June 10, 2020 13:58
@dblock
Copy link
Member

dblock commented Jun 10, 2020

Ah right 馃槃

PR it. Let's at least ack your work in CHANGELOG!

@yogeshjain999
Copy link
Contributor Author

Thanks @dblock 馃嵒 Created PR #526 for it.

dblock added a commit that referenced this pull request Jun 11, 2020
Small amendments for Hash#merge with IndifferentAccess
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants